Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Value Change Callback #237

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Value Change Callback #237

wants to merge 3 commits into from

Conversation

mb706
Copy link
Contributor

@mb706 mb706 commented Jun 28, 2019

See tests for how exactly this could work. The idea is to have a $callbacks slot of functions that get called whenever $values changes; the functions get called in turn and return a possibly modified version of the values to be set. This would close #231.

@codecov
Copy link

codecov bot commented Jun 28, 2019

Codecov Report

Merging #237 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   94.55%   94.61%   +0.05%     
==========================================
  Files          20       20              
  Lines         496      501       +5     
==========================================
+ Hits          469      474       +5     
  Misses         27       27
Impacted Files Coverage Δ
R/ParamSetCollection.R 95.77% <100%> (ø) ⬆️
R/ParamSet.R 97.24% <100%> (+0.09%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f4c9fb...8daebd9. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 28, 2019 •
edited
Loading

Codecov Report

Merging #237 into master will increase coverage by 0.82%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #237      +/-   ##
==========================================
+ Coverage   94.55%   95.37%   +0.82%     
==========================================
  Files          20       20              
  Lines         496      584      +88     
==========================================
+ Hits          469      557      +88     
  Misses         27       27
Impacted Files Coverage Δ
R/ParamSetCollection.R 95.77% <100%> (ø) ⬆️
R/ParamSet.R 98.24% <100%> (+1.1%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f4c9fb...139c1fb. Read the comment docs.

@jakob-r
Copy link
Member

jakob-r commented Jul 3, 2019

Essentially a forced trafo?

@mb706
Copy link
Contributor Author

mb706 commented Jul 3, 2019

It allows for side-effects from changin parameter values. My use-case is changing one ParamSet member of a ParamSetCollection according to a parameter value (see the "callbacks on ParamSetCollection" test at the end of test_ParamSet.R in the PR).

@berndbischl
Copy link
Member

lets at least discuss this in toulouse, i am unsure wether this is a good thing

Base automatically changed from master to main January 25, 2021 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ParamSets that contain ParamSets of some parameter values
3 participants
@mb706 @jakob-r @berndbischl